Make nominatim endpoint modifiable#385
Make nominatim endpoint modifiable#385nhirokinet wants to merge 2 commits intoProject-OSRM:gh-pagesfrom
Conversation
|
Hi, thank you for review! You've set auto merge and approved, so once the conflicts are resolved, I think the PR will be automatically merged. |
|
Please resolve the merge conflict |
Most of API endpoints can be modified via leaflet_options.js, which is mentioned in README as a target of manual modify. But nominatim endpoint cannot be modified by this way. As nominatim can be self-hosted, I think it should be modifiable in configuration.
Head branch was pushed to by a user without write access
|
I resolved the merge conflict and tested again. |
There was a problem hiding this comment.
Pull request overview
This PR makes the Nominatim (forward/reverse geocoding) endpoint configurable, aligning it with other backend endpoints that can be customized via src/leaflet_options.js and the build-time replacement script.
Changes:
- Add a
nominatim.pathconfiguration entry tosrc/leaflet_options.js. - Wire the configured Nominatim URL through
src/index.jsinto the geocoder factory. - Allow build-time substitution of the Nominatim endpoint via
scripts/replace.js.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/leaflet_options.js | Introduces a configurable Nominatim base URL in the exported options. |
| src/index.js | Passes the configured Nominatim URL into the geocoder initialization. |
| src/geocoder.js | Updates coordPreserving to build a Nominatim geocoder using a configurable serviceUrl. |
| scripts/replace.js | Adds env-driven replacement for the Nominatim endpoint in generated config files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| geocoder.coordPreserving = function(nominatimUrl) { | ||
| var nominatim = L.Control.Geocoder.nominatim({serviceUrl: nominatimUrl}); |
There was a problem hiding this comment.
nominatimUrl will never be undefined or empty as there is default value. Do we need it?
| // Leaflet uses LatLng | ||
| else options = options.replace('38.8995,-77.0269', latlng) | ||
| } | ||
| if (NOMINATIM_ENDPOINT) options = options.replace('https://nominatim.openstreetmap.org/', NOMINATIM_ENDPOINT) |
There was a problem hiding this comment.
options is not provided option but managed in this repository, so I think it is not needed to accept the one without trailing slash(/).
| }, | ||
| nominatim: { | ||
| path: 'https://nominatim.openstreetmap.org/' | ||
| } |
There was a problem hiding this comment.
test/leaflet_options.test.js looks like not having this kind of test for other parameters
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
I've applied or replied to AI-generated comments. |
Hi, thank you for nice product.
How about make nominatim endpoint modifiable?
I tried to make my code fit to current code, but if there is comment please feel free to comment about modification.
(The following is the commit message)
Most of API endpoints can be modified via leaflet_options.js, which is mentioned in README as a target of manual modify.
But nominatim endpoint cannot be modified by this way. As nominatim can be self-hosted, I think it should be modifiable in configuration.